Bug Fix: GitHub Usernames are Case Insensitive #241

megan07 · 2019-06-14T19:22:54Z

Addressing #196 where GitHub usernames were being treated as case-sensitive in the diff.
Before fix:

Step 1 error: After applying this step, the plan was not empty:

        DIFF:

        DESTROY/CREATE: github_team_membership.test_team_membership
          etag:     "W/\"52d84b121c000efbcf03e7e877c4fbf7\"" => "<computed>"
          id:       "3289152:Mbang-github2" => "<computed>"
          role:     "member" => "member"
          team_id:  "3289152" => "3289152"
          username: "mbang-github2" => "Mbang-github2" (forces new resource)

Step 1 error: After applying this step, the plan was not empty:

        DIFF:

        DESTROY/CREATE: github_membership.test_org_membership
          etag:     "W/\"ef0b40d063d184967e965b7d8af83466\"" => "<computed>"
          id:       "mbang-github-acc-test:mbang-github2" => "<computed>"
          role:     "member" => "member"
          username: "mbang-github2" => "Mbang-github2" (forces new resource)

    Error: Provider produced inconsistent result after apply

        When applying changes to
        github_repository_collaborator.test_repo_collaborator, provider "github"
        produced an unexpected new value for was present, but now absent.

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.

FAIL

After fix:

=== RUN   TestAccGithubTeamMembership_basic
--- PASS: TestAccGithubTeamMembership_basic (13.25s)
PASS

=== RUN   TestAccGithubMembership_basic
--- PASS: TestAccGithubMembership_basic (4.08s)
PASS

=== RUN   TestAccGithubRepositoryCollaborator_basic
--- PASS: TestAccGithubRepositoryCollaborator_basic (10.31s)
PASS

I also tested that if the user DOES update the casing in the GitHub UI, a terraform refresh will pick that up.

nywilken

@megan07 this looks good so far. I took a quick pass and left a couple of comments. I'll circle back later today.

github/resource_github_membership_test.go

nywilken · 2019-06-21T14:47:27Z

github/resource_github_membership_test.go

@@ -140,3 +162,18 @@ var testAccGithubMembershipConfig string = fmt.Sprintf(`
    role = "member"
  }
 `, testCollaborator)
+
+func testAccGithubMembershipConfig_caseInsensitive() string {
+	otherCase := []rune(testCollaborator)


If I get the intent right, you are modifying the collaborator username, by toggling the case of the first rune, to test case insensitivity functionality. Is that correct? If so what about changing the function signature to take in an input, and just pass the collaborator name with the expected case.

func testAccGithubMembershipConfig_caseInsensitive(memberName string) { return fmt.Sprintf(` resource "github_membership" "test_org_membership" { username = "%s" role = "member" } `, memberName) }

This way in the acceptance test you can run two TestSteps:

Steps: []resource.TestStep{ { Config: testAccGithubMembershipConfig_caseInsensitive(strings.ToUpper(testCollaborator)), Check: resource.ComposeTestCheckFunc( testAccCheckGithubMembershipExists("github_membership.test_org_membership", &membership), ), }, { Config: testAccGithubMembershipConfig_caseInsensitive(strings.ToLower(testCollaborator)), Check: resource.ComposeTestCheckFunc( testAccCheckGithubMembershipExists("github_membership.test_org_membership", &membership), ), }, },

github/resource_github_repository_collaborator.go

nywilken

@megan07 I left a few comments for you, but this is otherwise good.

--- PASS: TestAccGithubMembership_basic (3.30s)
--- PASS: TestAccGithubMembership_caseInsensitive (2.84s)
--- PASS: TestAccGithubMembership_importBasic (2.79s)
--- PASS: TestAccGithubRepositoryCollaborator_basic (9.65s)
--- PASS: TestAccGithubRepositoryCollaborator_importBasic (9.17s)

github/resource_github_repository_collaborator.go

paultyng

Some minor comments, but otherwise LGTM

paultyng · 2019-06-24T19:37:13Z

github/resource_github_membership_test.go


 	"github.com/google/go-github/v25/github"
 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/terraform"
 )

 func TestAccGithubMembership_basic(t *testing.T) {
+	if testCollaborator == "" {
+		t.Skip("Skipping because length of `GITHUB_TEST_COLLABORATOR` is not set")


think this is a copy/paste typo for the message

paultyng · 2019-06-24T19:42:34Z

github/resource_github_membership_test.go

+
+func testAccGithubMembershipTheSame(orig, other *github.Membership) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		if *orig.URL != *other.URL {


should this also test to make sure at least one isn't null/empty? or should two empties be equal?

I think, for the intent of the test, two empties would be equal. I can't think of a time when this would be empty here, but if they were to both be empty, I think it means the case change didn't have an effect.

paultyng · 2019-06-24T19:53:05Z

github/resource_github_membership_test.go

+	var otherMembership github.Membership
+
+	oc := []rune(testCollaborator)
+	if unicode.IsUpper(oc[0]) {


not sure if its possible, but could 0 ever be a number or symbol in a username?

kjolley-zainar · 2023-12-15T18:49:23Z

I'm still seeing this:

% tf --version
Terraform v1.6.6
on darwin_amd64

% tf plan
...
# module.github.github_repository_collaborators.myenv will be updated in-place
  ~ resource "github_repository_collaborators" "myenv" {
        id             = "myenv"
      ~ invitation_ids = {} -> (known after apply)
        # (1 unchanged attribute hidden)

      - user {
          - permission = "pull" -> null
          - username   = "foo" -> null
        }
      - user {
          - permission = "pull" -> null
          - username   = "bar" -> null
        }
      + user {
          + permission = "pull"
          + username   = "Foo"
        }
      + user {
          + permission = "pull"
          + username   = "Bar"
        }

        # (4 unchanged blocks hidden)
    }

Megan Bang added 5 commits June 14, 2019 10:12

update usernames to case insensitive

e4a5d4e

flatten stuff

a9ba1cd

Undo branch protection work

0b784bb

fix imports

08b732c

remove changes to org block

a5b71f6

ghost added the size/M label Jun 14, 2019

update tests

e4375c3

ghost added size/L and removed size/M labels Jun 14, 2019

mbfrahry and others added 3 commits June 17, 2019 07:35

Fixing panic when testCollaborator isn't set

b56a29c

Remove unneccessary test check

ff7d46d

fix repository collaborator test

19b5f88

megan07 requested a review from a team June 20, 2019 19:53

nywilken reviewed Jun 21, 2019

View reviewed changes

github/resource_github_repository_collaborator.go Outdated Show resolved Hide resolved

github/resource_github_repository_collaborator.go Outdated Show resolved Hide resolved

Megan Bang and others added 3 commits June 24, 2019 12:03

updates after review comments

a68fc4c

updates after review comments

793befc

Merge branch 'master' into f-username-case

b76fe4f

megan07 requested a review from nywilken June 24, 2019 18:52

Megan Bang added 3 commits June 24, 2019 13:55

indent

7e8fa40

merge with master

2b1b424

adding test collaborator check to test

37fed31

paultyng approved these changes Jun 24, 2019

View reviewed changes

Megan Bang added 2 commits June 24, 2019 15:07

removing 'length of' from skip message

34e594c

update tests for cases for first char is not a letter

d995f54

megan07 requested a review from paultyng June 24, 2019 21:26

paultyng approved these changes Jun 25, 2019

View reviewed changes

megan07 merged commit efbf3f2 into master Jun 26, 2019

megan07 deleted the f-username-case branch June 26, 2019 15:48

megan07 mentioned this pull request Jun 26, 2019

GitHub usernames are case-insensitive and should be treated that way #196

Closed

kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022

Bug Fix: GitHub Usernames are Case Insensitive (integrations#241)

581d4d0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug Fix: GitHub Usernames are Case Insensitive #241

Bug Fix: GitHub Usernames are Case Insensitive #241

megan07 commented Jun 14, 2019 •

edited

Loading

nywilken left a comment

nywilken Jun 21, 2019

nywilken left a comment

paultyng left a comment

paultyng Jun 24, 2019

paultyng Jun 24, 2019

megan07 Jun 24, 2019

paultyng Jun 24, 2019

kjolley-zainar commented Dec 15, 2023

Bug Fix: GitHub Usernames are Case Insensitive #241

Bug Fix: GitHub Usernames are Case Insensitive #241

Conversation

megan07 commented Jun 14, 2019 • edited Loading

nywilken left a comment

Choose a reason for hiding this comment

nywilken Jun 21, 2019

Choose a reason for hiding this comment

nywilken left a comment

Choose a reason for hiding this comment

paultyng left a comment

Choose a reason for hiding this comment

paultyng Jun 24, 2019

Choose a reason for hiding this comment

paultyng Jun 24, 2019

Choose a reason for hiding this comment

megan07 Jun 24, 2019

Choose a reason for hiding this comment

paultyng Jun 24, 2019

Choose a reason for hiding this comment

kjolley-zainar commented Dec 15, 2023

megan07 commented Jun 14, 2019 •

edited

Loading